Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Overlay index fetch issue #82

Closed

Conversation

terryluan12
Copy link
Contributor

Should resolve #78 and one of the additional bugs mentioned in #80

@terryluan12
Copy link
Contributor Author

As mentioned in #80, there were issues when initializing the Stat objects. Looking further into it, I found that the dirEntries was called to get the directory entries. However, it simply iterates through the keys of the Index. Since the Index object is in the process of being created when this is called, not all keys which would otherwise be in the final Index object are there when it's called.

I also found the bug which caused the double unlocking bug. This occurs if the program locks a path 3 times.
On it's first lock, it would creates a Promise 'A'. Then when it locks a second time, it awaits Promise 'A'. Finally on it's third time, it would also await Promise 'A'. When the program unlocks the lock (resolving Promise 'A'), both threads which are awaiting 'A' start running at the same time, which causes the bug.

@james-pre
Copy link
Member

@terryluan12 Some changes have been made. Primarily, the Mutex logic has been folded into LockedFS. Please consult the release notes for v0.14.0.

@terryluan12
Copy link
Contributor Author

Ah, didn't even notice it, thanks for the heads up! I just merged + re-added the changes

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes relate to the issue you mentioned in #80, please refer to my other comment.

@@ -26,7 +26,7 @@ export class LockedFS<FS extends FileSystem> implements FileSystem {
/**
* The current locks
*/
private locks: Map<string, MutexLock> = new Map();
private locks: Map<string, [MutexLock]> = new Map();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a tuple with a single MutexLock. Do you mean an array, MutexLock[]?

src/backends/locked.ts Show resolved Hide resolved
@@ -83,8 +88,8 @@ export class LockedFS<FS extends FileSystem> implements FileSystem {
}

// Non-null assertion: we already checked locks has path
this.locks.get(path)!.resolve();
this.locks.delete(path);
const res = this.locks.get(path)!.shift();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, please inline: this.locks.get(path)!.shift().resolve();

src/backends/locked.ts Show resolved Hide resolved
@james-pre
Copy link
Member

james-pre commented Jul 2, 2024

As mentioned in #80, there were issues when initializing the Stat objects. Looking further into it, I found that the dirEntries was called to get the directory entries. However, it simply iterates through the keys of the Index. Since the Index object is in the process of being created when this is called, not all keys which would otherwise be in the final Index object are there when it's called.

This should have been resolved in ec86a6a. Please let me know if the issue still happens- you will need to run make-index again. I am against using a regex since it is relatively slow. If one was to have 10,000 files for example, creating a new regex for each one slows things down.

@james-pre
Copy link
Member

@terryluan12 Any updates on this?

@james-pre
Copy link
Member

@terryluan12 Any updates on this?

@terryluan12
Copy link
Contributor Author

Hi @james-pre, so sorrry about the lack of update! Got busy with some real life stuff, and didn't see the comments. Just got a replacement for my previous laptop, and will get to work on zen-fs, and finish up this issue ASAP

@terryluan12
Copy link
Contributor Author

Hey @james-pre, I've been taking a look at the double unlocking issue, and I've gotten an idea of why it's bugging. Specifically, it occurs when you have two processes waiting on a locked promise. Once the promise resolves, both processes will continue at the same time. Thus they'll both call addLock, and one promise will override the others, causing the weird double unlocking. Thinking about it, I think one possible solution is to implement a queuing system. What are your thoughts?

@james-pre
Copy link
Member

@terryluan12,

Thanks for your dedication to this.

My thinking is that we should try to avoid a queue system.

It should work like this:

  • call asynchronous methods a and b on the same path without using await,
    • i.e.
     	a(path);
     	b(path);
  • a locks on the path, which adds the lock
  • b tries to lock on the path, though it must wait for a to finish (in LockedFS.lock)
  • a performs its operations
  • a's scope is left, MutexLock[Symbol.dispose] runs, unlocking the path
  • b then performs its operations
  • b's scope is left, MutexLock[Symbol.dispose] runs, unlocking the path

I think e387889 may fix something since now the promise won't resolve until the lock is actually cleared.

@terryluan12
Copy link
Contributor Author

No worries at all, thanks for allowing me to help!

I think there's been a misunderstanding. To clarify, the issue occurs in the following scenario, when there are 3 locks; for example A, B, and C.
In this case, if A locks first, and then B and C lock (in any order), then both B and C are now waiting on A's lock to release.

When A then releases, both B and C start again at the same time. I believe this is the unintended behaviour, where they both start again at the same time.

@james-pre
Copy link
Member

Ah, this makes more sense now! Thanks for the clarification.

Maybe we could look at a wrap and replace, so when B attempts the lock, it replaces the current lock with one that resolves when B is done. This wouldn't impact A since it holds the lock, and doesn't update the reference (i.e. the A's MutexLock[Symbol.dipose] resolves only the lock for A).

@james-pre
Copy link
Member

@terryluan12 Now that the mutex issue has been solved, can I close this PR?

@terryluan12
Copy link
Contributor Author

Yep, absolutely!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Overlay with Fetch/IndexedDB backend issue
2 participants